Initial factory reset code #1588
Conversation
ckyrouac
force-pushed
the
factory-reset
branch
from
387f495 to
ab75caf
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new experimental bootc install reset command, which appears to function as a factory reset. The implementation is well-structured, refactoring existing deployment logic to use a new MergeState enum, which is a clean way to handle different deployment sources. The changes also include new utilities for managing stateroots. I've found a couple of issues that need to be addressed before merging.
|
I think we'll want tests at least before merging, and supporting that with tmt is probably going to be an excellent driver for the feature. I haven't dug in but I think tmt is relying on injecting ssh keys w/cloud-init, so it might Just Work to do a reset there? |
ckyrouac
force-pushed
the
factory-reset
branch
4 times, most recently
from
209e3c0 to
b45e272
Compare
Signed-off-by: Colin Walters <[email protected]>
This is a nondestructive variant of `to-existing-root`. Signed-off-by: Colin Walters <[email protected]>
Also cleans up some bugs from rebasing previous commits.
ckyrouac
force-pushed
the
factory-reset
branch
from
b45e272 to
a0cd215
Compare
Updated with a basic test. |
This is currently used to signal to `bootc status` that the deployment is a factory reset and doesn't support soft reboot. Otherwise, the soft reboot code will throw an exception due to mismatched kargs. Signed-off-by: ckyrouac <[email protected]>
ckyrouac
force-pushed
the
factory-reset
branch
from
a0cd215 to
2ad17c6
Compare
| let deployment_dir = ostree.deployment_dirpath(&staged); | ||
| let deployment_dir = std::path::Path::new(deployment_dir.as_str()); | ||
| if deployment_dir.exists() { | ||
| let marker_path = deployment_dir.join(".bootc-factory-reset"); |
There was a problem hiding this comment.
Let's have clear const for things like this with an explanation there.
But backing up a second, in the ostree side we have "deployment backing" directories already where we can put metadata.
Though of course we should also be thinking about how this would work with the composefs backend.
Hmmm hmm. Here's an idea, how about we use the deployment's etc directory for state like this instead? I think we could save e.g. system.bootc.merged_from xattr or so?
Basically I lean towards extended attributes over "stamp files" for metadata.
BTW though do note the intersection here with etc.transient - I wonder if we should make that more visible in status output too. If that's enabled each upgrade is already like a reset of /etc.
| let deployment_dir = sysroot.deployment_dirpath(deployment); | ||
| let deployment_dir = std::path::Path::new(deployment_dir.as_str()); | ||
| let is_factory_reset = | ||
| deployment_dir.exists() && deployment_dir.join(".bootc-factory-reset").exists(); |
There was a problem hiding this comment.
This is where we want a const that is shared
| if is_factory_reset { | ||
| // Factory reset deployments don't support soft reboots | ||
| // this is primarily because the kargs validation will fail when checking for soft reboot | ||
| // compatibility in the ostree code, which will cause bootc status and upgrade to fail. |
There was a problem hiding this comment.
I don't understand why we need to special case this. Why is bootc status failing exactly?
If we're changing (usually dropping) kernel arguments won't the existing karg check work?
There was a problem hiding this comment.
As a followup - we talked about this in a live chat and there seems to be a libostree bug here where the staged deployment is missing ostree= and that trips an assertion in the soft reboot checks.
Ideally we root cause and fix that.
But we may be able to work around this by detecting when there's no kargs in the staged deployment?
|
Just saw the demo for this. A few random thoughts. It'd be nice to be able to have different reset levels. One is just resetting Should this use stateroots at all? This would be another thing to implement in the composefs backend (or maybe that code and concept already exist there?). But also it's quite common for One alternative though is basically to just move everything in (There's still the case of submounts in |
This is a rebased version of #1389 with some fixes on top to make
bootc install reset --experimentalwork with the latest soft reboot code. I'd like to get this in hidden behind--experimental, then iterate on it. I plan to add some e2e tests as a followup but I can work on including them as part of this PR if you want.